Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove reflection usage for most SKObject #1193

Closed
wants to merge 4 commits into from

Conversation

Gillibald
Copy link
Contributor

Description of Change

This PR tries to remove the performance impact that comes from reflection usage within the SKObject.GetObject factory

Bugs Fixed

  • Related to issue #

Provide links to issues here. Ensure that a GitHub issue was created for your feature or bug fix before sending PR.

API Changes

List all API changes here (or just put None), example:

Added:

  • string Class.Property { get; set; }
  • void Class.Method();

Changed:

  • object Cell.OldPropertyName => object Cell.NewPropertyName

Behavioral Changes

Describe any non-bug related behavioral changes that may change how users app behaves when upgrading to this version of the codebase.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Updated documentation

@Gillibald
Copy link
Contributor Author

Only objects that are using internal static TSkiaObject GetObject<TSkiaObject, TSkiaImplementation> create their instances via reflection. I wasn't sure how the concrete implementations of SKStream are working.

I would love to know what you think about these changes @mattleibow

@mattleibow
Copy link
Contributor

mattleibow commented Mar 30, 2020

I have been thinking about this for some time now, so it is great that you are having a look too.

I am not a total fan of duplicating the logic in each class, especially when it comes to changes - such as #1200. Is there a way we can use delegates or another means to specify a constructor? Basically, instead of duplicating code, or using reflection, we pass a cached delegate in (this code will be in the new HandleDictionary, but just a demo):

class SKObject {
    internal static TSkiaObject GetObject<TSkiaObject, TSkiaImplementation> (
        IntPtr handle, bool owns = true, bool unrefExisting = true, bool refNew = false, Func<IntPtr, bool, TSkiaObject> ctorDel = null)
    {
        if (GetInstance(out instance))
            return instance;
        if (ctorDel != null)
            return ctorDel(handle);
        // do reflection
    }
} 

Then in the usage, we can do something similar to what you did with adding a method to each type:

class SKSurface {
    public SKCanvas Canvas => SKCanvas.GetObject(sk_surface_canvas());
}

class SKCanvas {
    internal static SKCanvas GetObject (IntPtr handle, bool owns = true, bool unrefExisting = true, bool refNew = false) =>
        SKObject.GetObject<SKCanvas>(handle, owns, unref, ref, (h, o) => new SKCanvas(h, o));
}

This way, there is no code duplication (better for maintenance) and no reflection (perf).

This is just me thinking here, so you may spot some issues or maybe improve on this?

@Gillibald
Copy link
Contributor Author

Your solution is indeed more maintainable. Will adapt the delegate approach in this PR and benchmark it.

@Gillibald
Copy link
Contributor Author

Closing this in favor of the concurrency PR. Will work in this when that is finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants